Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

T6773: RFC-2136 support for Kea DHCP4 server #4153

Open
wants to merge 16 commits into
base: current
Choose a base branch
from
Open

T6773: RFC-2136 support for Kea DHCP4 server #4153

wants to merge 16 commits into from
+706 −11

Conversation

abukharov
Copy link

@abukharov abukharov commented Oct 12, 2024 •
edited
Loading

Change Summary

This PR introduces support for RFC-2136 DDNS updates in Kea DHCP4 server.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

https://vyos.dev/T6773

Related PR(s)

vyos/vyos-documentation#1561

Component(s) name

dhcp-server

Proposed changes

Introduces new configuration parameters under service dhcp-server and the logic to translate it into Kea 2.4.1 configuration language.

How to test

Set up a Technitium DNS in a container inside VyOS. Set up DDNS updates as follows:

set service dhcp-server dynamic-dns-update send-updates enable
set service dhcp-server dynamic-dns-update use-conflict-resolution enable
set service dhcp-server dynamic-dns-update tsig-key mydomain-net algorithm hmac-sha256
set service dhcp-server dynamic-dns-update tsig-key mydomain-net secret eWF5YW15bGl0dGxla2V5IQ==
set service dhcp-server dynamic-dns-update forward-domain mydomain.net key-name mydomain-net
set service dhcp-server dynamic-dns-update forward-domain mydomain.net dns-server 1 address '172.18.0.254'
set service dhcp-server dynamic-dns-update forward-domain mydomain.net dns-server 1 port 1053

Get the DHCP server to issue some leases and check if the DNS server gets updated.

Smoketest result

sh-5.2# ./test_service_dhcp-server.py
test_dhcp_dynamic_dns_update (__main__.TestServiceDHCPServer.test_dhcp_dynamic_dns_update) ... ok
test_dhcp_exclude_in_range (__main__.TestServiceDHCPServer.test_dhcp_exclude_in_range) ... ok
test_dhcp_exclude_not_in_range (__main__.TestServiceDHCPServer.test_dhcp_exclude_not_in_range) ... ok
test_dhcp_high_availability (__main__.TestServiceDHCPServer.test_dhcp_high_availability) ... ok
test_dhcp_high_availability_standby (__main__.TestServiceDHCPServer.test_dhcp_high_availability_standby) ... ok
test_dhcp_multiple_pools (__main__.TestServiceDHCPServer.test_dhcp_multiple_pools) ... ok
test_dhcp_on_interface_with_vrf (__main__.TestServiceDHCPServer.test_dhcp_on_interface_with_vrf) ... ok
test_dhcp_relay_server (__main__.TestServiceDHCPServer.test_dhcp_relay_server) ... ok
test_dhcp_single_pool_options (__main__.TestServiceDHCPServer.test_dhcp_single_pool_options) ... ok
test_dhcp_single_pool_options_scoped (__main__.TestServiceDHCPServer.test_dhcp_single_pool_options_scoped) ... ok
test_dhcp_single_pool_range (__main__.TestServiceDHCPServer.test_dhcp_single_pool_range) ... ok
test_dhcp_single_pool_static_mapping (__main__.TestServiceDHCPServer.test_dhcp_single_pool_static_mapping) ... ok

----------------------------------------------------------------------
Ran 12 tests in 38.710s

OK
sh-5.2#

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@abukharov abukharov requested a review from a team as a code owner October 12, 2024 03:27
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 12, 2024 •
edited
Loading

👍
No issues in PR Title / Commit Title

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 12, 2024 •
edited
Loading

✅ No issues found in unused-imports check.. Please refer the workflow run

@abukharov abukharov mentioned this pull request Oct 12, 2024
Open
1 task
dmbaturin
dmbaturin requested changes Oct 15, 2024
View reviewed changes
Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is good, I left some ideas for improvements in the review.

interface-definitions/include/dhcp/ddns-settings.xml.i Outdated Show resolved Hide resolved
interface-definitions/include/dhcp/ddns-settings.xml.i Outdated Show resolved Hide resolved
interface-definitions/include/dhcp/ddns-settings.xml.i Outdated Show resolved Hide resolved
interface-definitions/include/dhcp/ddns-settings.xml.i Outdated Show resolved Hide resolved
interface-definitions/service_dhcp-server.xml.in Outdated Show resolved Hide resolved
interface-definitions/service_dhcp-server.xml.in Outdated Show resolved Hide resolved
interface-definitions/service_dhcp-server.xml.in Show resolved Hide resolved
python/vyos/template.py Show resolved Hide resolved
interface-definitions/include/dhcp/ddns-dns-server.xml.i Show resolved Hide resolved
data/templates/dhcp-server/kea-dhcp4.conf.j2 Show resolved Hide resolved
@abukharov
Copy link
Author

Did a few cosmetic things. Please let me know what you think about generating that config, DNS servers list and the port 53001 for d2 and I'll get it fixed within a few days.

sarthurdev
sarthurdev requested changes Oct 18, 2024
View reviewed changes
Copy link
Member

@sarthurdev sarthurdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be an issue in the XML definition stopping the CI tests. I will try find time later to pinpoint the failing syntax.

@abukharov
Copy link
Author

Seems to be an issue in the XML definition stopping the CI tests. I will try find time later to pinpoint the failing syntax.

Was able to figure it out. Should be fine now.

@abukharov
Copy link
Author

Everything seems to be fixed now, all tests are green

@abukharov
T6773: dhcp-server: ddns: Had to redo parts of the configuration lang…
fac667f 
…uage due to how Kea treats set/unset options in scopes
@abukharov
Copy link
Author

So I started productionising this setup, which meant setting up more sophisticated configurations for DHCP. And I quickly realised that this config language wasn't doing what was actually needed. The main problem is that in Kea it makes a difference if an attribute is set to false or not set at all. It's to do with how the DDNS settings cascade to nested scopes.

Let's say you set force-update on the parent scope and don't set anything in the networks/subnets. A valueless leafnode, when it's not set, will result in a false setting, which will override the parent setting. However, in Kea the idea is that when it's not set to either true or false (i.e. the attribute is missing from the configuration file), the setting is inherited from the parent scope, so there is a difference between an attribute missing and set to false.

I've re-implemented the behavioral settings in a way that they can be explicitly set to enabled or disabled, or left out/deleted from config, the latter will result in a removal of the attribute from the output config file. This way I got the whole thing to function properly, even though the language has become a bit more heavyweight.

If you have any ideas about how to make it nicer and easier to understand for the end user, I'm open to suggestions.

@github-actions GitHub Actions
Copy link

CI integration 👍 passed!

Details

CI logs

  • CLI Smoketests (no interfaces) 👍 passed
  • CLI Smoketests (interfaces only) 👍 passed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed
  • TPM tests 👍 passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zdc zdc Awaiting requested review from zdc zdc is a code owner automatically assigned from vyos/reviewers

@jestabro jestabro Awaiting requested review from jestabro jestabro is a code owner automatically assigned from vyos/reviewers

@sever-sever sever-sever Awaiting requested review from sever-sever sever-sever is a code owner automatically assigned from vyos/reviewers

@fett0 fett0 Awaiting requested review from fett0 fett0 is a code owner automatically assigned from vyos/reviewers

@sarthurdev sarthurdev Awaiting requested review from sarthurdev

@dmbaturin dmbaturin Awaiting requested review from dmbaturin

Requested changes must be addressed to merge this pull request.

Assignees

@abukharov abukharov

Labels
current rebase
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abukharov @dmbaturin @sarthurdev